Skip to content

core, txpool, miner, eth/gasprice, internal/ethapi: reserved blockspace follow-ups - #2376

Open
kamuikatsurgi wants to merge 25 commits into
reserved-blockspace-block-buildingfrom
kamuikatsurgi/reserved-blockspace-followups
Open

core, txpool, miner, eth/gasprice, internal/ethapi: reserved blockspace follow-ups#2376
kamuikatsurgi wants to merge 25 commits into
reserved-blockspace-block-buildingfrom
kamuikatsurgi/reserved-blockspace-followups

Conversation

@kamuikatsurgi

@kamuikatsurgi kamuikatsurgi commented Aug 27, 2026

Copy link
Copy Markdown
Member

Summary

Follow-up hardening on top of the core Reserved Blockspace protocol change (#2302): value-only-balance txpool admission and fallback-fee handling (POS-3671), an aggregate reserved-sender occupancy cap to prevent normal-sender starvation (POS-3681), gas-price-oracle/eth_feeHistory exclusion of fee-free reserved transactions plus a new normalGasUsedRatio field (POS-3675/3676), chain/reserved/* and worker/reserved/* observability metrics (POS-3679), and execution-path produce/import determinism coverage at the fork boundary across the serial and both BlockSTM processors (POS-3672). Also carries the wire-format reconciliation needed when the Austin hard fork changed the header extra-data shape mid-implementation, and two correctness fixes found via live devnet testing (see below). Also fixes a txpool-only correctness/security gap found during a same-day security review of the occupancy-cap change: a same-nonce transaction replacement could grow to txMaxSize while skipping the reserved-occupancy cap check entirely, letting a reserved sender occupy several times their intended pool share (ad75b96ad).

A combined delivery-and-test-status primer for both this branch and the base protocol branch, written for the still-open production go/no-go decision:
Reserved Blockspace - Delivery & Test Status

Executed tests

  • Full unit + -race suites across every touched package, and all 9 tests/bor reserved integration tests green.
  • Two full live devnet trials on a 9-node Kurtosis topology covering every node kind (producers, witness producer, stateless validators/RPC with and without BlockSTM parallel import, archive) - full write-up: Reserved Blockspace Devnet Trial.
    • Round 1 reproduced the spec's core behaviors live, then found a critical issue while exercising registry governance for the first time on a live chain: growing the registry could permanently wedge stateless nodes, and with stateless validators in the mix this cascaded into a full network production halt via lost Heimdall span-rotation quorum (registryreader.BuildSnapshot read the registry against a state copy whose witness never reached the real per-block witness - cf9f3e82c). Also found feeMode=1 clients were mining fee-free identically to feeMode=0, contradicting the contract's own documented semantics (124087f55).
    • Round 2 rebuilt from both fixes and re-ran the identical, riskier topology (stateless validators included) through the same trigger, a rapid-mutation stress pass, and two full node cold-restarts, with zero divergence - plus a deeper pass on the registry contract's own access-control and boundary semantics (client-admin delegation, exact quota-cap boundaries, invalid-input reverts, a 21-client/67.5M-gas registry with no measurable timing impact).
  • Not yet covered: a naturally-occurring reorg around a reserved-tx block, and a standing automated e2e/upgrade CI harness (both scripted-but-manual so far, not checked into CI).

Rollout notes

Consensus-affecting (gated behind Bor.ReservedBlockspaceBlock, same as the base branch). No operator-facing config changes beyond the existing ReservedMaxOccupancyPercent txpool knob (default 50%, CLI-wired). Both devnet-found issues above are fixed on this branch; production readiness still depends on the smart-contract team's audited registry contract and genesis-contracts parity, neither of which is part of this PR - see the primer's "still outstanding" section.

Node upgrade ordering: this PR adds a new header field (ReservedCapacity, alongside the existing ReservedGasUsed) to BlockExtraData/BlockExtraDataPostAustin. Like any hard-fork field addition, every node needs the upgraded binary before any of them start producing past the fork block - a pre-upgrade node attempting to parse a post-fork header carrying this field will fail. Nothing is live on any network yet, so this is purely a reminder for whoever runs the next devnet round or upgrade rehearsal: roll out the binary to all nodes first, in line with the existing Bor.ReservedBlockspaceBlock hard-fork gate.

…ool occupancy

Reserved-blockspace senders (POS-3674) are eviction-immune in the txpool with
no aggregate ceiling: a client with enough whitelisted addresses could occupy
unbounded pending+queued slots, starving normal fee-paying senders network-wide
since the reserved set is registry-derived and therefore consensus-uniform.

Adds a combined pending+queued reservedOccupancy counter on LegacyPool, capped
by config.ReservedMaxOccupancyPercent (default 50% of GlobalSlots+GlobalQueue).
Two-layered by design: an incremental O(1) counter updated at every mutation
site, plus a periodic from-scratch recompute in reset() that self-heals any
drift every reorg cycle. Admission is gated with a new
ErrReservedOccupancyExceeded; a reorg-time backstop trims the largest reserved
account via the same prque idiom truncatePending already uses. The new config
knob is wired through internal/cli/server like its sibling pool-size settings.

POS-3681.
… suggestions and expose normal-region gas-used ratio
…or a clean compare

# Conflicts:
#	consensus/bor/bor.go
#	consensus/bor/bor_test.go
#	core/reserved_validation_test.go
#	core/types/block.go
#	core/types/block_test.go
#	miner/worker.go
…or a clean compare

# Conflicts:
#	core/blockchain.go
#	core/blockchain_test.go
#	eth/api_debug.go
#	miner/worker.go
The still-in-pool invariant was judged against both nodes' pools while
mined-ness came only from node0's canonical chain. The producing node
drops a tx from its pool the moment its own head includes it, which can
be several hundred ms before node0 imports that block, so the assertion
raced with block propagation (and with tip-fork reinjection windows).

A tx now counts as healthy on a node if it is pending in that node's
pool or canonical on that node's own chain, and a drop only fails the
test after persisting across consecutive polls. A genuine balance
eviction is permanent, so it still trips the threshold.
…he reserved set

The registry defines feeMode 1 (routed: fee paid, credited to the producer)
as reserved for a future external-block-producer world; the spec's zero-fee
handling applies to feeMode 0 only. The reader carried FeeMode as metadata
but never consulted it, so a routed client's senders inherited the free-mode
waiver end to end and mined at effectiveGasPrice 0, identically to feeMode 0.

Resolve fee mode at snapshot build, the single choke point every consumer
derives from: non-free clients stay out of the effective set, so their
senders pay standard fees, their quotas leave EffectiveCapacity, and no
downstream surface (EVM waiver, txpool, sequencing, header stamping) needs
its own gate. The Snapshot's now-meaningless FeeMode plumbing is removed.

registrytest gains a CreateClient helper and caller-provided state (the
latter also serves the witness-completeness tests in the next commit) to pin
the exclusion against the real registry bytecode.
…d witnesses

registryreader.BuildSnapshot read the registry against a throwaway
statedb.Copy(), and Copy deep-clones the attached witness: every trie node
the read touched landed in the discarded clone and never reached the witness
shipped to peers. A block's witness only carried the registry's storage when
the block's own transactions happened to touch it, so the first
transaction-free block after a registry shape change (an ordinary
createClient) permanently wedged every stateless node with "missing trie
node" for the registry account; with stateless validators holding voting
power, span rotation lost quorum and the whole chain halted.

Extract the copy-read-collect protocol the span and state-sync reads already
inlined twice in consensus/bor into state.StateDB.ReadIsolated, and route
all three call sites through it: run the reads on a reset copy, record them
into the live witness (StartPrefetcher + IntermediateRoot), and re-register
them on the live state so they enter its FlatDiff read surface
(PropagateReadsTo). The shared helper also stops the copy's prefetcher on
error paths (previously leaked at the bor.go sites), skips the witness
machinery when none is being produced, and detaches the witness while
copying instead of cloning it just to replace the clone.

A registrytest regression test pins the contract end to end: a witness
produced during a snapshot build must let a consumer rebuild the identical
snapshot from the witness's node set alone, exactly as a stateless verifier
does.
Five pipeline tests reassigned engine, exitCh, or speculativeWorkCh on a
live worker whose background goroutines read those fields concurrently,
failing go test -race deterministically. Stop the worker's goroutines first
(idempotently, so the fixture cleanup can still run) or build a bare worker
directly with the wrapped engine where no goroutines are needed. The
closed-exitCh swaps become the real thing: a stopped worker's exitCh is
genuinely closed.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.22449% with 279 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.56%. Comparing base (880bd7c) to head (ad75b96).

Files with missing lines Patch % Lines
core/txpool/legacypool/legacypool.go 79.74% 46 Missing and 2 partials ⚠️
core/rawdb/reserved_txs_freezer.go 61.34% 32 Missing and 14 partials ⚠️
core/blockchain.go 76.15% 28 Missing and 3 partials ⚠️
core/state/statedb.go 0.00% 27 Missing ⚠️
internal/ethapi/api.go 54.16% 19 Missing and 3 partials ⚠️
internal/ethapi/bor_api.go 14.28% 14 Missing and 4 partials ⚠️
core/rawdb/reserved_txs.go 78.87% 11 Missing and 4 partials ⚠️
core/txpool/legacypool/list.go 81.81% 6 Missing and 4 partials ⚠️
consensus/bor/registryreader/reader.go 70.00% 9 Missing ⚠️
miner/worker.go 78.04% 5 Missing and 4 partials ⚠️
... and 12 more

❌ Your patch check has failed because the patch coverage (77.22%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@                          Coverage Diff                           @@
##           reserved-blockspace-block-building    #2376      +/-   ##
======================================================================
+ Coverage                               55.33%   55.56%   +0.23%     
======================================================================
  Files                                     917      919       +2     
  Lines                                  167127   167993     +866     
======================================================================
+ Hits                                    92480    93351     +871     
+ Misses                                  69124    69082      -42     
- Partials                                 5523     5560      +37     
Files with missing lines Coverage Δ
consensus/misc/eip1559/eip1559.go 94.70% <100.00%> (-1.92%) ⬇️
core/blockchain_reader.go 60.43% <100.00%> (+0.64%) ⬆️
core/parallel_state_processor.go 75.35% <100.00%> (+12.67%) ⬆️
core/rawdb/ancient_scheme.go 0.00% <ø> (ø)
core/rawdb/database.go 15.66% <100.00%> (ø)
core/rawdb/freezer.go 54.29% <100.00%> (+4.15%) ⬆️
core/state_processor.go 67.29% <100.00%> (+3.91%) ⬆️
core/txpool/legacypool/queue.go 94.97% <100.00%> (ø)
core/txpool/validation.go 18.64% <100.00%> (+14.01%) ⬆️
core/types/receipt.go 57.66% <100.00%> (+1.59%) ⬆️
... and 30 more

... and 24 files with indirect coverage changes

Files with missing lines Coverage Δ
consensus/misc/eip1559/eip1559.go 94.70% <100.00%> (-1.92%) ⬇️
core/blockchain_reader.go 60.43% <100.00%> (+0.64%) ⬆️
core/parallel_state_processor.go 75.35% <100.00%> (+12.67%) ⬆️
core/rawdb/ancient_scheme.go 0.00% <ø> (ø)
core/rawdb/database.go 15.66% <100.00%> (ø)
core/rawdb/freezer.go 54.29% <100.00%> (+4.15%) ⬆️
core/state_processor.go 67.29% <100.00%> (+3.91%) ⬆️
core/txpool/legacypool/queue.go 94.97% <100.00%> (ø)
core/txpool/validation.go 18.64% <100.00%> (+14.01%) ⬆️
core/types/receipt.go 57.66% <100.00%> (+1.59%) ⬆️
... and 30 more

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines +2553 to +2557
func (pool *LegacyPool) effectiveCost(addr common.Address, tx *types.Transaction) *big.Int {
if pool.isReserved(addr) {
return tx.Value()
}
return tx.Cost()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q on this one: effectiveCost gives value only pricing just because the sender is a registered reserved client, without checking if this specific tx's gas actually fits their quota.
I understand that the comment above says this is intentional, but doesn't that mean a reserved sender can get fee-free pricing AND the eviction protection on a tx that was never going to execute fee-free anyway? Feels like it opens the door to spamming the pool pretty cheaply. Might be worth a quota check at admission time too, wdyt?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this is intentional, matches the spec directly - §8.1 waives the balance check unconditionally for registered senders, no quota check. AccountSlots/AccountQueue plus the new occupancy cap already bound this even without an admission-time quota check - the spec's own §8.2 calls out a per-client cap as the natural follow-up if spam shows up in practice, which is basically what the occupancy cap already is. don't think we need this right now.

Comment on lines +1056 to +1069
pendingList := pool.pending[from]

// Reserved-blockspace occupancy cap: reject outright, before any
// Discard/eviction attempt, if this sender is reserved and admitting the
// transaction would occupy a genuinely new slot (as opposed to a same-
// nonce replacement, which leaves combined occupancy unchanged) that
// pushes aggregate reserved occupancy over its cap. Runs unconditionally,
// independent of overall pool fullness — unlike the Underpriced exemption
// below, which only applies once the pool is already globally full.
if reserved && pool.isNewReservedSlot(pendingList, from, tx) {
if pool.reservedOccupancy+1 > pool.reservedOccupancyCap() {
stage0Duration = time.Since(stage0Time)
return false, ErrReservedOccupancyExceeded
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reservedOccupancyCap() is computed against GlobalSlots+GlobalQueue, which is the same denominator the pool uses for its real slot based fullness check (with numSlots(tx)). However, every place that bumps reservedOccupancy just does +1/-1 per tx, never numSlots(tx). So a reserved sender sending big calldata txs (multiple slots each) would only count as "1" against a cap that's measured in slots? Keep me honest here, but could someone blow past the intended 50% pool share pretty easily, right? Maybe worth a check

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, real bug - fixed. was bumping by flat 1 per tx while the cap's computed in slots. also found a nastier version of the same bug while fixing it: same-nonce replacements skipped the cap check entirely (assumed occupancy-neutral, which held under flat counting but not once tx size can differ), so a sender could replace cheap txs with maximal-size ones at trivial cost and blow past the cap 4x, repeatably. fixed both, added tests confirming they fail on the old code and pass now.

return nil, err
}
if !c.Active || c.EffectiveFrom > effectiveAt {
if !c.Active || c.EffectiveFrom > effectiveAt || c.FeeMode != FeeModeFree {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like non-free (feeMode!=0) clients still get read from the registry every single block before getting filtered out. Not a big deal today since it's gated behind whoever controls the whitelist, but if that list grows with a bunch of non-free addresses, seems like unnecessary per-block work for addresses that can never be reserved anyway. Maybe we should filter earlier in the read path? Not sure how much this matters in practice tho

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't think this is fixable in a meaningful way - you need the read to know the fee mode in the first place, so there's no way to filter earlier than right after it comes back. cost's the same for every whitelisted address regardless of mode, a non-free client doesn't add anything extra, it just doesn't make it into the map.

Comment thread eth/gasprice/gasprice.go

var reserved map[common.Hash]struct{}
for _, receipt := range receipts {
if isReservedReceipt(receipt) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in feehistory, I think this might hit on the state-sync txs, because it's inferring "reserved" from EffectiveGasPrice == 0, but the state-sync tx also reports 0. Could we end up skewing eth_feeHistory a bit on blocks that have both? Seems like internal/ethapi/api.go uses the actual index table instead, which seems safer. Shall we reuse that here too?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, state-sync's effectiveGasPrice is hardcoded to 0 so it does match. checked the actual impact though and it's a no-op today - state-sync's gasUsed is 0 (no weight in feehistory either way) and its tip is 0 (already below the ignoreUnder floor in the sampler regardless). real imprecision, zero practical effect right now. fixing it properly means widening OracleBackend's interface for a currently-inert bug, so I'd rather leave it documented and pick it up if it ever actually matters.

Comment thread core/types/block.go
payload := h.Extra[ExtraVanityLength : len(h.Extra)-ExtraSealLength]

if chainConfig.Bor != nil && chainConfig.Bor.IsAustin(h.Number) {
var blockExtraData BlockExtraDataPostAustin

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add the ReservedCapacity as a new field, does this mean an old node can't parse a header once that field shows up? Probably fine since nothing's live yet and we have a HF, but might be worth an addition in the PR description / docs reminding whoever runs the next devnet round that every node needs the new binary before any of them start producing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, added a note to the PR description about upgrade ordering. no code change needed here, this is just normal hard-fork rollout behavior.

Comment thread core/blockchain.go
Comment on lines 2607 to +2635
@@ -2612,9 +2627,12 @@ func (bc *BlockChain) InsertReceiptChain(blockChain types.Blocks, receiptChain [
headers = append(headers, block.Header())
}

// Write all chain data to ancients.
// Write all chain data to ancients. These blocks arrive via receipt
// sync rather than local execution, so there is no reserved-tx
// classification to carry; the nil entries make writeAncientBlock
// record that explicitly.
td := bc.GetTd(first.Hash(), first.NumberU64())
writeSize, err := rawdb.WriteAncientBlocks(bc.db, blockChain, receiptChain, borReceipts, td)
writeSize, err := rawdb.WriteAncientBlocks(bc.db, blockChain, receiptChain, borReceipts, make([]rlp.RawValue, len(blockChain)), td)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do blocks that come in via receipt sync get reservedTxIndexes hardcoded to nil? I mean it would make sense, as no local execution is there to derive it from, but doesn't that mean EffectiveGasPrice for reserved txs on those blocks is just permanently wrong, with no way to backfill? Might be a non-issue if we never actually run that sync mode across the reserved-blockspace range, but wanted to double check.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confirmed, real gap but scoped narrowly - only fallback-fee-in-quota reserved txs are affected (zero-fee ones come out correct by accident since their own fee fields are already 0). also inherent to how receipt-sync works today, the reserved-tx table is bor-local and never gossiped so there's nothing to backfill from without a wire-protocol change. think this is best left as a documented limitation for now rather than fixed here.

@marcello33

Copy link
Copy Markdown
Collaborator

Shall we improve diffguard? Also worth merging develop back to unlock the kurtosis related tests

…ng' into kamuikatsurgi/reserved-blockspace-followups

# Conflicts:
#	miner/pipeline_session_test.go
#	miner/pipeline_test.go
reservedOccupancy bumped by a flat 1 per transaction at every mutation
site, but reservedOccupancyCap is computed in the pool's own slot unit
(GlobalSlots+GlobalQueue), the same one pool.all.Slots() uses for real
fullness. A handful of large-calldata reserved transactions could
occupy far more of the pool's actual capacity than the cap was meant
to admit - counting as "1" each while consuming several slots -
undermining the exact aggregate-starvation defense
ReservedMaxOccupancyPercent exists to provide, just via few big
transactions instead of many small ones.

Weight every mutation site (the admission gate, promotion/removal
loss, and the bulk fairness/demotion/truncation paths) by numSlots(tx)
instead of a flat 1, and rename reservedCount to reservedSlots to
match.

reservedSlots now reads a new totalslots field on list, maintained
incrementally in Add/subTotals the same way totalcost/totalvalue
already are, instead of summing list.Flatten() on every call -
Flatten nonce-sorts and copies the whole list once its cache is
invalidated, which the reorg-time occupancy backstop would otherwise
pay on every single eviction iteration (removeTx invalidates the
cache it just read).

Slot-weighting also exposed a real cap bypass in a pre-existing
assumption: isNewReservedSlot treated any same-nonce replacement as
occupancy-neutral, which was true under flat per-transaction counting
(1 out, 1 in) but is false once transactions can differ in slot count.
A reserved sender could fill their cap with minimal transactions, then
replace each at a trivial fallback-fee cost with a maximal-size
(txMaxSize) transaction at the same nonce - every replacement skipping
the cap check entirely, since "same nonce" short-circuited it - ending
up occupying several times the intended share while reservedOccupancy
kept reporting exactly the cap. Replaced isNewReservedSlot with
reservedSlotAt, which returns the incumbent transaction (if any) so
both the admission gate and the two replacement call sites (add's
pending-replace branch, enqueueTx's queue-replace branch) can compute
and apply the real numSlots(new)-numSlots(old) delta, gating and
costing a replacement exactly like any other admission.

New regression tests pin both the capacity-header-style slot-weighting
fix and the replacement-delta fix: a single sender's 3-slot
transactions must get rejected once their combined slot count would
exceed the cap well before their transaction count does, an oversized
same-nonce replacement must be rejected against the cap rather than
skipped outright, and a successful size-changing replacement must
update occupancy by the real delta rather than leaving it stale.
Confirmed all three fail on the pre-fix code and pass with the fix.
Shared test fixtures (bigZeroFeeTx, bigReplacementTx) reuse
zeroFeeTxWithGasAndData rather than duplicating the transaction-
building literal.
@kamuikatsurgi

Copy link
Copy Markdown
Member Author

merged develop into both branches, pushed - should pick up the kurtosis-pos pin bump and unblock e2e-tests.

on diffguard - agree it's worth doing, but I'd rather run it as its own pass. it's currently flagging 17 survived mutants across 8 files, that's a bigger job than a comment-response round.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants